8383809: [type-classes] Prototype UInt128#2420
Conversation
|
👋 Welcome back rgiulietti! A progress list of the required criteria for merging this PR into |
|
@rgiulietti This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jddarcy) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
openjdk
Bot
added
ready
Webrevs
|
|
Subject to change and discussion:
|
| /** | ||
| * Returns the instance representing {@code v}. | ||
| * | ||
| * @param v must meet 0 ≤ {@code v} < 2<sup>{@link #SIZE}</sup>. |
There was a problem hiding this comment.
As a small issue, I don't know if having "<=" as a single character, as it seems to be here, goes against general encoding rules for JDK sources.
There was a problem hiding this comment.
JDK source code has switched to UTF-8 some time ago.
| * @param y the 2nd parameter | ||
| * @return the result described above. | ||
| */ | ||
| static boolean lessThan(long x, long y) { |
There was a problem hiding this comment.
Is it intentional that this method is not public?
Having a @see link to Long.compareUnsigned, if the obvious relationship between the methods holds, would be helpful.
There was a problem hiding this comment.
The idea is that the public API point is compareTo().
But it's no deal to make the static comparison methods public as well.
There was a problem hiding this comment.
Ah, I know see what you mean.
Addressed in new commit.
|
There's a |
|
/integrate |
|
@rgiulietti |
| if (lc == 'x') { | ||
| return parse(s, i, end, i, 16, '_', -1); | ||
| } | ||
| if (lc == 'b') { | ||
| return parse(s, i, end, i, 2, '_', -1); | ||
| } | ||
| return parse(s, begin, end, i, 8, '_', c); |
There was a problem hiding this comment.
This can be a pattern matching switch:
return switch (lc) {
case 'x' -> parse(s, i, end, i, 16, '_', -1);
case 'b' -> parse(s, i, end, i, 2, '_', -1);
default -> parse(s, begin, end, i, 8, '_', c;
};There was a problem hiding this comment.
Indeed.
Maybe in a followup PR.
Thanks
|
/sponsor |
|
Going to push as commit 628aacc. |
openjdk
Bot
removed
ready
|
@jddarcy @rgiulietti Pushed as commit 628aacc. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
3 participants
Prototype a fully functional
unsigned int128type as a Valhalla value class.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2420/head:pull/2420$ git checkout pull/2420Update a local copy of the PR:
$ git checkout pull/2420$ git pull https://git.openjdk.org/valhalla.git pull/2420/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2420View PR using the GUI difftool:
$ git pr show -t 2420Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2420.diff
Using Webrev
Link to Webrev Comment